Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CheckHyphens #484

Merged
merged 6 commits into from
Jul 17, 2019
Merged

Implement CheckHyphens #484

merged 6 commits into from
Jul 17, 2019

Conversation

nox
Copy link
Contributor

@nox nox commented Feb 11, 2019

Fixes #483

This change is Reviewable

@nox
Copy link
Contributor Author

nox commented Feb 11, 2019

I need to write a test but lunch time is approaching. :)

@nox
Copy link
Contributor Author

nox commented Feb 11, 2019

I made idna::domain_to_ascii not check hyphens, and idna::uts46::to_ascii continue to check hyphens by default. Not sure that this is the right trade-off to make.

@SimonSapin
Copy link
Member

UTS 46 does not specify defaults for its flags, but checking hyphens is what previous versions did:

The URL standard explicitly sets CheckHyphens to false. So yes, I think these are the right defaults.

I think I’d rather not add a new Config type in the pubic API as long as Flags also exist. What do you think of keeping it private for now? I’ve filed #486 about cleaning this up on the next breaking change batch, and referenced it from #463.

@SimonSapin SimonSapin mentioned this pull request Feb 14, 2019
@nox
Copy link
Contributor Author

nox commented Feb 17, 2019

I don't think making Config public is an issue, even when Flags goes away in a future version Config will still be here. I tried to make the change anyway, and I can't use pub(crate) because that stabilised in 1.18.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relaxed parsing mode
3 participants